Implemented selective disable of tools to spare with the initial schema size#1035
Implemented selective disable of tools to spare with the initial schema size#1035carlos-alm merged 10 commits intooptave:mainfrom
Conversation
Greptile SummaryThis PR implements selective MCP tool disabling via a new Confidence Score: 5/5This PR is safe to merge — the feature is well-scoped, correctly tested, and introduces no regressions. No P0 or P1 issues found. The normalization logic is sound, the enabledToolNames gate is consistent with how TOOL_HANDLERS was already keyed (plain lowercase names), DEFAULTS initialize the array so deep-merge always yields a string[], and the updated tests cover both the list and call handlers end-to-end. Prior review threads have been addressed. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[startMCPServer] --> B[loadConfig]
B --> C[config.mcp.disabledTools]
C --> D[buildToolList multiRepo, disabledTools]
D --> E[buildDisabledToolSet\nnormalizeToolName each entry]
E --> F{includeTool?\n!disabled.has\nnormalizeToolName tool.name}
F -- excluded --> G[tool omitted]
F -- included --> H[enabledTools array]
H --> I[enabledToolNames Set\ntool.name for each]
H --> J[tools/list handler\nreturns enabledTools]
I --> K[tools/call handler]
K --> L{enabledToolNames.has name ?}
L -- no --> M[isError: Unknown tool: name]
L -- yes --> N[validateMultiRepoAccess\ncheck args.repo]
N --> O[TOOL_HANDLERS.get name\ndispatch to handler]
Reviews (8): Last reviewed commit: "Merge branch 'main' into feat/mcp-disabl..." | Re-trigger Greptile |
| // Apply config-based MCP page-size overrides | ||
| const config = options.config || loadConfig(); | ||
| initMcpDefaults(config.mcp?.defaults ? { ...config.mcp.defaults } : undefined); | ||
| const disabledTools = config.mcp?.disabledTools ? [...config.mcp.disabledTools] : undefined; |
There was a problem hiding this comment.
Ternary
undefined branch is unreachable due to empty-array truthiness
DEFAULTS now sets disabledTools: [] as string[], so after deep-merging, config.mcp.disabledTools will always be an array (at worst []). An empty array is truthy in JavaScript, so config.mcp?.disabledTools ? [...config.mcp.disabledTools] : undefined will always take the truthy branch — the undefined branch can never be reached.
| const disabledTools = config.mcp?.disabledTools ? [...config.mcp.disabledTools] : undefined; | |
| const disabledTools = [...(config.mcp?.disabledTools ?? [])]; |
There was a problem hiding this comment.
Fixed in 242944b — replaced the ternary with [...(config.mcp?.disabledTools ?? [])] so disabledTools is always a string[]. Verified the only consumer (buildToolList → buildDisabledToolSet) already normalizes undefined/[] identically (disabledTools || [] produces an empty Set), so behavior is unchanged.
Codegraph Impact Analysis8 functions changed → 7 callers affected across 3 files
|
Impact: 1 functions changed, 1 affected
The 'list_repos' tool is excluded from 'enabledToolNames' when 'multiRepo' is false, so 'createCallToolHandler' rejects single-repo 'list_repos' calls with 'Unknown tool' before they ever reach 'validateMultiRepoAccess'. That made the 'name === "list_repos"' branch unreachable. Drop the dead branch and the now-unused 'name' parameter; existing test 'rejects list_repos in single-repo mode' covers the early-rejection path.
|
Addressed Greptile's summary callout about the unreachable 'list_repos' branch in 'validateMultiRepoAccess'. Cleaned up in b37d464 — dropped the dead branch and the now-unused 'name' parameter. The existing test 'rejects list_repos in single-repo mode' (tests/unit/mcp.test.ts:1058) already covers the early-rejection path via 'enabledToolNames'. |
- Reorder createCallToolHandler so the disabled-tool check runs before validateMultiRepoAccess, giving config-disabled tools a consistent "Unknown tool" response regardless of multi-repo state. - Mark CodegraphConfig.mcp.disabledTools optional to match the runtime guard (config.mcp?.disabledTools) and tolerate partial configs. - Update list_repos single-repo test to reflect the unified "Unknown tool" path. - Apply biome formatting to normalizeToolName.
- README: short subsection under Configuration with example and link. - docs/guides/mcp-tool-filtering.md: extensive guide covering motivation, name normalization, runtime behavior, full tool catalog, and recipes.
Per review feedback, the previous guide was too narrowly scoped. Replace with docs/guides/configuration.md covering every key in DEFAULTS: file selection, build, query, embeddings, llm, search, ci, manifesto, check, coChange, analysis, community, structure, risk, display, mcp. README points to the new guide and keeps the short MCP tool-filtering example for the PR's headline feature.
Impact: 1 functions changed, 1 affected
The 'list_repos' tool is excluded from 'enabledToolNames' when 'multiRepo' is false, so 'createCallToolHandler' rejects single-repo 'list_repos' calls with 'Unknown tool' before they ever reach 'validateMultiRepoAccess'. That made the 'name === "list_repos"' branch unreachable. Drop the dead branch and the now-unused 'name' parameter; existing test 'rejects list_repos in single-repo mode' covers the early-rejection path.
0df784b to
8842024
Compare
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Fixed of #1023
Fixes #1022